-
-
Notifications
You must be signed in to change notification settings - Fork 429
Better gRPC error handling #1251
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this, it's a great improvement that's very much needed. I just have some doubt about the returning an rpc.UploadResponse even when the status.Status might contain errors.
64b9066 to
adfa4a2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this considered a breaking change to the API?
If so, does the standard process for documenting it need to be done still?
https://arduino.github.io/arduino-cli/latest/CONTRIBUTING/#pull-request-checklist
|
🤦🏼 right, we should prepare a migration guide. I'll write down some notes later today... |
| // library and possibly adjust the case of the name to match a library in the index | ||
| func ParseLibraryReferenceArgAndAdjustCase(instance *rpc.Instance, arg string) (*LibraryReferenceArg, error) { | ||
| libRef, err := ParseLibraryReferenceArg(arg) | ||
| libRef, _ := ParseLibraryReferenceArg(arg) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we fix this by handling this err instead of ignoring it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably already clear, but I did this because this assignment of err was ignored and the error type from the declaration was incompatible with the change of lib.LibrarySearch()'s return type from error to *status.Status.
If it's to be ignored, I think it's best to make that explicit by using _. But if an error return from ParseLibraryReferenceArg() is significant in this context, then of course it's better to handle it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, the err was ignored also before this PR (it was overwritten without being checked). This should be fixed in another PR BTW.
|
The translation files must be updated, the rest is good. I'll approve as soon that's fixed. 👍 |
|
After trying the current solution for a bit I found two annoying problems:
PROPOSAL To address the two problems above, while rebasing this PR on master, I'd like to try a different approach:
|
|
Yes, I 100% agree with this new design. I think it also make it easier to handle errors internally and recover them gracefully. |
4a6bd7c to
c146dae
Compare
Requested changes have been made. Thanks!
Co-authored-by: per1234 <accounts@perglass.com>
777b59c to
63dd036
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All in all it's a great improvement but there are some small things to fix.
Co-authored-by: Silvano Cerza <3314350+silvanocerza@users.noreply.github.com>
This is a proposal to uniform gRPC error handling.
For now only the
uploadcommand has been ported, as a proof-of-concept. It will be extended to all thearduino-clicode base if approved.This PR, once fully implemented, should:
status.Statusinstead of anerror(theStatusmessage is how the gRPC transports errors on RPC calls).codesStatus.detailsfield (for example theUploadRequiresProgrammerError)